Autolink basic and built-in types in typespecs #802
Conversation
| put_placeholder(form, string, placeholders) | ||
|
|
||
| {name, arity} in @basic_types -> | ||
| url = @elixir_docs <> "elixir/" <> @basic_types_page |
There was a problem hiding this comment.
TODO: this is not gonna work for hexdocs.pm/elixir/master, v1.5.0, need to find better way of handling this
There was a problem hiding this comment.
FWIW e.g. EEx master [1] links to Elixir stable, so it seems solution above is consistent with that. Then, we'd only need to detect whether we build docs for elixir app itself in which case this would be a relative: url = @basic_types_page
WDYT?
There was a problem hiding this comment.
Yeah, I think we need to pipe it though the same logic we link functions today. Then when we change one, everything else works.
There was a problem hiding this comment.
@josevalim here's another idea: what if we "define" basic & built-in types in the Kernel module?
Another advantage is it magically works from IEx:
To make it work, I had to comment out some code in Kernel.Typespec:
# if builtin_type?(name, arity) do
# compile_error(caller, "type #{name}/#{arity} is a builtin type and it cannot be redefined")
# endThere's similar code in Erlang too, and so I can't yet do:
@type term() :: any()One way to solve it is to write it like this instead:
@builtin_type type ::
any()
| none()
| ...
| builtin_type()
@builtin_type builtin_type ::
term()
| arity()
| ...
@basic_type any()
@builtin_type term() :: any()The idea would be that @basic_type doesn't actually define the type but makes it accessible by tooling.
Not sure if we could represent https://hexdocs.pm/elixir/typespecs.html#literals section here though.
There was a problem hiding this comment.
Oh, one other caveat with this: not sure if it'd be technically correct to have types on the Kernel because perhaps they're considered part of the runtime rather.
Should we pad the placeholder by the amount of letters in which word? So "foobar" should become "ppp1"? Something like: ? |
|
ppp1 sounds good! |
|
|
||
| {name, arity} in @builtin_types -> | ||
| source = get_source(Kernel, [], lib_dirs) | ||
| url = "#{source}#{enc_h(inspect Kernel)}.html#t:builtin/0" |
There was a problem hiding this comment.
Why enc_h(inspect Kernel)? Don't we know before hand what is the output of this operation?
Also wouldn't be better to consistently link to the typespecs page?
|
@josevalim a follow up on the experiment that defines types in the Kernel: if you pull this ex_doc branch, and apply wojtekmach/elixir@394259a you'll see that way we could drop typespec page, and document types in the Kernel. On that branch I also included There are a few caveats, like the literal type needs to be defined in typedoc, and Elixir throws some warnings about useless types. But yeah, if this is the direction woth pursuing let me know and I'll keep at it, otherwise I can go back to what I had initially in this PR to get it working with typespecs page, and consider moving this to Kernel in a PR to Elixir. |
|
@josevalim updated! Regarding moving basic types to Kernel, while there might be some merit to what I proposed, there's still too much other considerations (e.g. as you mentioned we don't want to expose literal type for others) so I'm parking it for now. |
![sourcelevel-bot[bot]](https://avatars.githubusercontent.com/in/597?s=60&v=4){kind=small}
| string = format_typespec_form(form, url) | ||
| put_placeholder(form, string, placeholders) | ||
|
|
||
|
|
There was a problem hiding this comment.
There should be no more than 1 consecutive blank lines.
|
Ebert has finished reviewing this Pull Request and has found:
You can see more details about this review at https://ebertapp.io/github/elixir-lang/ex_doc/pulls/802. |
|
❤️ 💚 💙 💛 💜 |
Ref #682 - however here we only handle typespecs in this PR
Ref elixir-lang-core: Anchor built-in types - however we just link to a section of that page and not to particular item
Example:
Before:
After:
Note: Pretty sure the different indentation is because the "After" version has more
_pX_placeholders and so it's shorter when formatting.